wasm2c: parametrize memory bounds checks on a per-memory basis #2507
Conversation
keithw
force-pushed
the
w2c-per-memory-strategy
branch
3 times, most recently
from
167eae8
to
985d89d
Compare
keithw
force-pushed
the
w2c-harmonize-types
branch
from
2ece28b
to
176ec50
Compare
keithw
force-pushed
the
w2c-per-memory-strategy
branch
from
985d89d
to
5604e96
Compare
keithw
force-pushed
the
w2c-harmonize-types
branch
from
176ec50
to
7bea8ee
Compare
keithw
force-pushed
the
w2c-per-memory-strategy
branch
from
5604e96
to
4e60e9c
Compare
keithw
force-pushed
the
w2c-harmonize-types
branch
2 times, most recently
from
d15bfb9
to
beb8809
Compare
|
|
||
| #define DEFINE_STORE(name, t1, t2) \ | ||
| static inline void name(wasm_rt_memory_t* mem, u64 addr, t2 value) { \ | ||
| MEMCHECK(mem, addr, t1); \ | ||
| static inline void name##_unchecked(wasm_rt_memory_t* mem, u64 addr, \ |
There was a problem hiding this comment.
Not a strong opinion, but would it be simpler to have something like?
#define DEFINE_STORE(name, t1, t2, MEMCHK, suffix) \
static inline void name##_##suffix(wasm_rt_memory_t* mem, u64 addr, t2 value) { \
MEMCHK(mem, addr, t1); \
t1 wrapped = (t1)value; \
wasm_rt_memcpy(MEM_ADDR_MEMOP(mem, addr, sizeof(t1)), &wrapped, sizeof(t1)); \
}
DEFINE_STORE(i32_store, u32, u32, MEMCHECK_DEFAULT32, _default32);
DEFINE_STORE(i32_store, u32, u32, MEMCHECK_GENERAL, _general);
There was a problem hiding this comment.
I guess I was trying to avoid the churn of duplicating every occurrence of DEFINE_LOAD/DEFINE_STORE/DEFINE_SIMD_LOAD_FUNC/DEFINE_SIMD_LOAD_LANE/DEFINE_SHARED_LOAD/etc. ... I think on balance the status quo is probably the less-bad but I don't feel that strongly either.
| memory->data = addr; | ||
| #else | ||
| memory->data = calloc(byte_length, 1); | ||
| if (WASM_RT_USE_MMAP && !is64) { |
There was a problem hiding this comment.
Nit: Might be cleaner to have
// Check if we should reallocate using mmap
#if WASM_RT_USE_MMAP
if(!is64) {
... // original code
return;
}
#endif
// Fallback to malloc
memory->data = calloc(byte_length, 1);
There was a problem hiding this comment.
Yeah... I agree this one is clearer but it seemed nice to have consistency with grow_memory_impl where the early-return is harder to do. :-/
keithw
force-pushed
the
w2c-harmonize-types
branch
2 times, most recently
from
487cb8a
to
326e3cb
Compare
keithw
force-pushed
the
w2c-per-memory-strategy
branch
from
4e60e9c
to
8abbc62
Compare
|
@sbc100 Did you want to review this one? @shravanrn would prefer to land #2506 and #2507 together, so I was planning to wait until everybody is comfortable with this one before merging either. |
(Sequenced behind #2506)
This PR allows "software-bounds-checked" memories and "guard-page-checked" memories to coexist in the same module.
It creates two versions of every memory operation: an unrestricted version (that works with any memory) and a
_default32version (for memories with default page size and i32 indexing).The unrestricted version calls
MEMCHECK_GENERAL, which does a 64-bit softwareRANGE_CHECKto check that the operation reads/writes within the bounds of the memory.The
_default32version callsMEMCHECK_DEFAULT32, which is the same as the oldMEMCHECK: if the runtime declaresWASM_RT_MEMCHECK_GUARD_PAGES, it will do nothing. Otherwise it will do a 32-bit softwareRANGE_CHECK(which seems to be one less instruction than the 64-bitRANGE_CHECK).This is a stepping stone to supporting custom-page-sizes (which will need to be software bounds-checked) (#2508).